Support delta brave_installer #5484

simonhong · 2020-05-07T08:22:42Z

In this PR, two new sub options are added to create_dist command like below.
last_chrome_installer is used to pass folder that contains previous version's uncompressed chrome.7z file.
Courgette generate diff file based on previous version's chrome.7z file with newly built chrome.7z.

npm run create_dist Release -- --build_delta_installer --last_chrome_installer="C:/build/installer"

This PR should be merged with brave/brave-browser#9667.

Resolves brave/brave-browser#9565

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

simonhong · 2020-05-08T09:08:17Z

patches/chrome-tools-build-win-create_installer_archive.py.patch

+   cmd = [lzma_exec,
+          'x',
+-         '-o"%s"' % temp_dir,
+         '-o%s' % temp_dir,


W/o this change, lzma_exec generates temp_dir under c:/ instead of using cwd's one.

Is it ever possible for temp_dir to have spaces? If so, you might try something like -o.\"%s"' % temp_dir

Yeah, thanks for catching. It would be much safer.
Changed to -o./"%s" % temp_dir.

I still don't understand why this is needed, how does it work for chrome?

After discussing with @bridiver , This is reverted to '-o%s' % temp_dir, because relative path in out doesn't have space.
and asked about this in chromium shared slack channel.

simonhong · 2020-05-08T09:09:10Z

patches/chrome-tools-build-win-create_installer_archive.py.patch

+ def GenerateDiffPatch(options, orig_file, new_file, patch_file):
+   if (options.diff_algorithm == "COURGETTE"):
+-    exe_file = os.path.join(options.last_chrome_installer, COURGETTE_EXEC)
+    exe_file = os.path.join(options.build_dir, COURGETTE_EXEC)


last_chrome_installer is directory that contains previous version's installer dir.
courgettes exe is generated in ./out.

Or we can avoid this patch if we copy courgette.exe from out/Release to folder indicated by last_chrome_installer before running build_delta_installer by Jenkins job. cc @mihaiplesa

This change is deleted.
courgette.exe will be copied from out/Release to last_chrome_installer by pipeline.

bridiver · 2020-05-13T20:43:01Z

patches/chrome-tools-build-win-create_installer_archive.py.patch

+   cmd = [lzma_exec,
+          'x',
+-         '-o"%s"' % temp_dir,
+         '-o./"%s"' % temp_dir,


why is this change needed?

Mentioned here - #5484 (comment)

I think the problem here is what we're passing in for last_chrome_installer because we shouldn't have to change the script itself. We should be passing in a a rebased value here https://github.com/brave/brave-core/pull/5484/files#diff-769a945034c01d9bc9d33ecfa819c902R18

sorry, I guess this is staging_dir, but in any case this should be running in the root build dir so there's no reason for it to end up in the root of the filesystem

bridiver · 2020-05-13T20:54:20Z

BUILD.gn

+    # before running create_dist with build_delta_installer.
+    # When mini_installer target generates diff installer patch, it assumes
+    # that courgette.exe is already in last_chrome_installer folder.
+    deps += [ "//courgette:copy_courgette_binaries" ]


I don't think this is the right place to put this. https://github.com/brave/brave-core/pull/5484/files#diff-769a945034c01d9bc9d33ecfa819c902R20 should be consolidated into a single dep on //brave/installer/mini_installer which has deps for courgette:copy_courgette_binaries conditionally on build_delta_installer and then on //brave instead of //brave:packed_resources and //brave/build/win:copy_exe

also this list is incomplete and can be eliminated by just making the target dependent on //brave https://github.com/brave/brave-core/pull/5484/files#diff-769a945034c01d9bc9d33ecfa819c902R19

bridiver

we should fix the path in gn, not patch the script https://github.com/brave/brave-core/pull/5484/files#r425343559

By using additional options (--build_delta_installer and --last_chrome_installer) with create_dist command, we can build delta installer on Windows.

bsclifton

++ as this was previously approved; changes to lzma_exec LGTM!

simonhong force-pushed the delta_installer_win branch 3 times, most recently from fddc682 to 7a2fed8 Compare May 8, 2020 09:04

simonhong commented May 8, 2020

View reviewed changes

simonhong self-assigned this May 8, 2020

simonhong added this to the 1.10.x - Nightly milestone May 8, 2020

simonhong force-pushed the delta_installer_win branch from 7a2fed8 to bfb1971 Compare May 8, 2020 09:29

simonhong marked this pull request as ready for review May 8, 2020 09:30

simonhong requested a review from bridiver as a code owner May 8, 2020 09:30

simonhong requested review from mkarolin, bsclifton and mihaiplesa May 8, 2020 09:30

mihaiplesa force-pushed the delta_installer_win branch 2 times, most recently from fac49c7 to 0d1d684 Compare May 12, 2020 20:48

mihaiplesa added the CI/skip Do not run CI builds (except noplatform) label May 12, 2020

simonhong force-pushed the delta_installer_win branch 2 times, most recently from b31cbb7 to e960833 Compare May 13, 2020 08:05

mihaiplesa approved these changes May 13, 2020

View reviewed changes

simonhong force-pushed the delta_installer_win branch 2 times, most recently from b4c5420 to f6a5108 Compare May 13, 2020 08:30

bridiver reviewed May 13, 2020

View reviewed changes

simonhong force-pushed the delta_installer_win branch 2 times, most recently from 9a2c6b2 to 73a3f12 Compare May 14, 2020 02:35

simonhong removed the CI/skip Do not run CI builds (except noplatform) label May 14, 2020

simonhong force-pushed the delta_installer_win branch 2 times, most recently from a981e06 to a72b7ee Compare May 14, 2020 04:29

mihaiplesa approved these changes May 14, 2020

View reviewed changes

bridiver requested changes May 14, 2020

View reviewed changes

simonhong force-pushed the delta_installer_win branch from a72b7ee to 28cfe06 Compare May 15, 2020 01:20

simonhong modified the milestones: 1.10.x - Beta, 1.11.x - Nightly May 15, 2020

simonhong force-pushed the delta_installer_win branch from 28cfe06 to ee0f732 Compare May 15, 2020 03:03

Support delta brave_installer

593ed79

By using additional options (--build_delta_installer and --last_chrome_installer) with create_dist command, we can build delta installer on Windows.

simonhong force-pushed the delta_installer_win branch from ee0f732 to 593ed79 Compare May 19, 2020 00:58

bridiver approved these changes May 19, 2020

View reviewed changes

bsclifton approved these changes May 19, 2020

View reviewed changes

simonhong merged commit 0f31032 into master May 19, 2020

simonhong deleted the delta_installer_win branch May 19, 2020 23:17

simonhong mentioned this pull request May 23, 2020

Add courgette target to generate courgette.exe in x86 #5636

Merged

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support delta brave_installer #5484

Support delta brave_installer #5484

simonhong commented May 7, 2020 •

edited

Loading

simonhong May 8, 2020 •

edited

Loading

bsclifton May 12, 2020 •

edited

Loading

simonhong May 13, 2020

bridiver May 14, 2020

simonhong May 15, 2020 •

edited

Loading

simonhong May 8, 2020

simonhong May 13, 2020 •

edited

Loading

simonhong May 13, 2020

bridiver May 13, 2020

simonhong May 14, 2020

bridiver May 14, 2020

bridiver May 15, 2020

bridiver May 13, 2020

bridiver May 13, 2020

bridiver left a comment

bsclifton left a comment

Support delta brave_installer #5484

Support delta brave_installer #5484

Conversation

simonhong commented May 7, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong May 8, 2020 • edited Loading

Choose a reason for hiding this comment

bsclifton May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonhong May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

simonhong commented May 7, 2020 •

edited

Loading

simonhong May 8, 2020 •

edited

Loading

bsclifton May 12, 2020 •

edited

Loading

simonhong May 15, 2020 •

edited

Loading

simonhong May 13, 2020 •

edited

Loading